Skip to content

Stop retrying permanently-LOADING family members without location sharing#221

Merged
timlaing merged 3 commits intotimlaing:mainfrom
TeroPihlaja:fix/findmyiphone-progress-tracking-retry
May 8, 2026
Merged

Stop retrying permanently-LOADING family members without location sharing#221
timlaing merged 3 commits intotimlaing:mainfrom
TeroPihlaja:fix/findmyiphone-progress-tracking-retry

Conversation

@TeroPihlaja
Copy link
Copy Markdown

@TeroPihlaja TeroPihlaja commented Apr 23, 2026

Problem

The _initialize_devices() retry loop polls until all family members leave the LOADING state or _MAX_REFRESH_RETRIES is exhausted. Family members with location sharing disabled (e.g. a Mac where the user has turned off Share My Location) return deviceFetchStatus: LOADING permanently — they never resolve.

This caused the loop to always exhaust all 5 retries and log:

Max retries reached when fetching family devices

on every single poll cycle, generating constant log noise and unnecessary API calls.

Root cause

The loop only tracked retry count, not whether any progress was made between retries. A permanently-stuck member looks identical to a slow-resolving member until all retries are spent.

The FMIP membersInfo payload contains no field indicating whether a member has location sharing enabled (only accountFormatter, firstName, lastName, deviceFetchStatus, useAuthWidget, isHSA, appleId), so the member cannot be filtered out by capability.

Fix

Track which member keys are LOADING between retries. If the set of LOADING members hasn't changed after a retry, those members will not resolve on further retries — stop early.

Retry 0: {MacBook} LOADING → retry (progress possible)
Retry 1: {MacBook} LOADING → same as before → stop

For a transient member that does resolve (e.g. an iPhone waking up):

Retry 0: {iPhone, MacBook} LOADING → retry (progress possible)
Retry 1: {MacBook} LOADING → set changed → retry (iPhone resolved!)
Retry 2: {MacBook} LOADING → same as before → stop

Result

  • Permanently-stuck members cause at most 2 retries (vs. always 5) before exiting cleanly
  • Transient members that do resolve still get up to _MAX_REFRESH_RETRIES attempts
  • No more "Max retries reached" log spam on every poll cycle
  • Tested with Home Assistant 2026.4.3, family account with one member (Mac) that has location sharing permanently disabled

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Co-authored with Claude Sonnet 4.6

…ring

The _initialize_devices() retry loop polled until all family members
left the LOADING state or _MAX_REFRESH_RETRIES was exhausted. Family
members with location sharing disabled return deviceFetchStatus: LOADING
permanently and never resolve, causing the loop to always exhaust all
retries and log "Max retries reached when fetching family devices" on
every single poll cycle.

Replace the fixed-count retry loop with a progress-tracking approach:
track which member keys are LOADING between retries and stop as soon as
the set stops changing. A member that is still LOADING after a retry
with no change will not resolve on further retries — give up on it.

Result:
- Transient LOADING members (e.g. Sampo's iPhone waking up) still get
  up to _MAX_REFRESH_RETRIES attempts to resolve
- Permanently-LOADING members (e.g. a Mac with location sharing off)
  cause at most one extra retry before the loop exits cleanly
- No more "Max retries reached" log spam on every poll cycle
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 23, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0be4bf80-e3bc-4560-a362-93dbdacee5d6

📥 Commits

Reviewing files that changed from the base of the PR and between a8c0927 and d62d0a1.

📒 Files selected for processing (1)
  • tests/services/test_findmyiphone.py

📝 Walkthrough

Summary by CodeRabbit

Bug Fixes

  • Improved family member device initialisation to detect when progress stalls and exit early, rather than exhausting all retry attempts. This reduces wait times when devices remain stuck in a loading state, providing a more responsive experience when syncing family member locations.

Walkthrough

The _initialize_devices method's family-device retry loop now computes the set of family members whose deviceFetchStatus is "LOADING" each iteration and stops early if the set becomes empty or unchanged from the previous retry, instead of always retrying until the maximum retry count.

Changes

Cohort / File(s) Summary
Device initialization retry logic
pyicloud/services/findmyiphone.py
Updated the family-device refresh loop to recompute the set of members with "LOADING" on each iteration and to exit early if no members are loading or if the LOADING set does not change between retries.
Tests for retry behaviour
tests/services/test_findmyiphone.py
Adjusted test_refresh_client_with_reauth_with_loading_no_complete to reflect the new early-stop behaviour by shortening mocked status progressions and reducing expected _refresh_client call count.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • Bugfix/fetch-devices-2 #135: Also modifies pyicloud/services/findmyiphone.py to refine family-device polling and refresh termination logic for members stuck in LOADING.

Poem

🐇 I nudge the loop, I check the set,
If none are loading, I don’t fret.
If nothing changes, I end the try—
Hopping on, no wasted sigh. ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely describes the main change: implementing early exit logic to stop retrying family members permanently stuck in LOADING state without location sharing.
Description check ✅ Passed The description is directly related to the changeset, providing clear problem statement, root cause analysis, fix explanation, and expected results that align with the code modifications.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 Pylint (4.0.5)
tests/services/test_findmyiphone.py

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@pyicloud/services/findmyiphone.py`:
- Around line 112-136: The loop in findmyiphone.py currently breaks early when
loading_keys == prev_loading_keys, but the failing test expects polling to
continue until _MAX_REFRESH_RETRIES is exhausted (i.e., multiple calls to
self._refresh_client). Remove the early-exit check (the "if loading_keys ==
prev_loading_keys: break" block) so the while loop will keep retrying up to
_MAX_REFRESH_RETRIES, continue updating prev_loading_keys = loading_keys each
iteration, and ensure self._refresh_client(locate=locate) is still called and
retries incremented as before.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 318360d0-6a94-4473-b379-5e16e2fb8623

📥 Commits

Reviewing files that changed from the base of the PR and between 7abec01 and a8c0927.

📒 Files selected for processing (1)
  • pyicloud/services/findmyiphone.py

Comment thread pyicloud/services/findmyiphone.py
@TeroPihlaja
Copy link
Copy Markdown
Author

Thanks for the detailed analysis @coderabbitai. This was already addressed in the follow-up commit d62d0a1 — the test_refresh_client_with_reauth_with_loading_no_complete test has been updated to expect 3 refresh calls (not 6) and the side_effect list trimmed to the 3 pairs actually consumed by the early-exit logic. The new docstring explains the expected behavior explicitly. Could you re-review with that commit included?

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 23, 2026

@TeroPihlaja: Thank you for the update! I'll kick off a fresh full review of the PR now, including commit d62d0a1 and all its changes to the test expectations and docstring.

✅ Actions performed

Full review triggered.

@TeroPihlaja
Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 24, 2026

Only users with a collaborator, contributor, member, or owner role can interact with CodeRabbit.

@TeroPihlaja TeroPihlaja marked this pull request as draft April 24, 2026 13:42
@TeroPihlaja TeroPihlaja marked this pull request as ready for review April 25, 2026 08:29
Copy link
Copy Markdown
Owner

@timlaing timlaing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TeroPihlaja - many thanks for your contribution

@timlaing timlaing merged commit 113e031 into timlaing:main May 8, 2026
13 checks passed
@TeroPihlaja TeroPihlaja deleted the fix/findmyiphone-progress-tracking-retry branch May 8, 2026 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants